Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial work towards supporting Groups v2 #73

Merged
merged 9 commits into from
Mar 4, 2021

Conversation

gferon
Copy link
Collaborator

@gferon gferon commented Mar 2, 2021

I open this PR so we can start discussing about the general direction we want to take for this change.

Expect: dirty code, hacks, ugly things and... being able to fetch the metadata of a group v2 given that you posses its master_key (which is part of what the client receives on every new message).

I'd mostly like to get some early review, and answers to the following questions:

  • Is the trait for CredentialsCache ok? (It's modeled after the cache used in the Java source code)
  • awc::Client sets the Authorization header twice if it's set at the client and request level, so I had to remove it from get_client and set it manually everywhere. This is error prone, but I couldn't think of another way
  • Are there too many .unwrap() in this change? 💝

TODO:

  • Decrypt the incoming data
  • Clean things up

Copy link
Member

@rubdos rubdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some things that caught my eye.

libsignal-service/src/gv2/api.rs Outdated Show resolved Hide resolved
libsignal-service/src/gv2/api.rs Outdated Show resolved Hide resolved
libsignal-service/src/gv2/api.rs Outdated Show resolved Hide resolved
libsignal-service/src/gv2/models.rs Outdated Show resolved Hide resolved
libsignal-service-actix/src/push_service.rs Outdated Show resolved Hide resolved
libsignal-service/src/configuration.rs Show resolved Hide resolved
Copy link
Member

@rubdos rubdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look to bad tbh.

Maybe think about the api.rs naming, because everything is API in the whole crate.

libsignal-service/src/gv2/operations.rs Outdated Show resolved Hide resolved
libsignal-service/src/gv2/operations.rs Outdated Show resolved Hide resolved
libsignal-service/src/gv2/models.rs Outdated Show resolved Hide resolved
libsignal-service/src/gv2/operations.rs Outdated Show resolved Hide resolved
Copy link
Member

@rubdos rubdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty review, that's not useful.

@gferon
Copy link
Collaborator Author

gferon commented Mar 3, 2021

Doesn't look to bad tbh.

Maybe think about the api.rs naming, because everything is API in the whole crate.

What about having everything in mod.rs? It's really not that much code.

@rubdos
Copy link
Member

rubdos commented Mar 3, 2021

Doesn't look to bad tbh.
Maybe think about the api.rs naming, because everything is API in the whole crate.

What about having everything in mod.rs? It's really not that much code.

That, or modeled after AccountManager we could have group_manager.rs?

@gferon gferon requested a review from rubdos March 4, 2021 14:15
@gferon gferon changed the title Draft: Initial work towards supporting Groups v2 Initial work towards supporting Groups v2 Mar 4, 2021
Copy link
Member

@rubdos rubdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some last very minor nitpicks. Thanks for the work!

libsignal-service/src/groups_v2/manager.rs Outdated Show resolved Hide resolved
libsignal-service-actix/src/push_service.rs Outdated Show resolved Hide resolved
libsignal-service/src/push_service.rs Outdated Show resolved Hide resolved
@gferon gferon merged commit d234dcb into whisperfish:master Mar 4, 2021
@gferon gferon deleted the groups-v2 branch March 4, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants